[EuiForm] specify default fullWidth with root component #6229

spalger · 2022-09-10T00:17:48Z

Summary

This PR is an attempt to use context in the <EuiForm> to support changing the default value of fullWidth in any form elements rendered within a given <EuiForm> element.

Checklist

Props have proper autodocs and playground toggles
Added documentation
Checked Code Sandbox works for any docs examples
Added or updated jest and cypress tests
Checked for breaking changes and labeled appropriately
A changelog entry exists and is marked appropriately
Validated that this has no UI impacts:
~~[ ] Checked in both light and dark modes~~
[ ] Checked in mobile
[ ] Checked in Chrome, Safari, Edge, and Firefox
~~[ ] Checked for accessibility including keyboard-only and screenreader modes~~
~~[ ] Updated the Figma library counterpart~~

…dth section

…form-layout-context

…w anymore

spalger · 2022-09-10T04:14:43Z

src/components/form/described_form_group/described_form_group.tsx

     * Default max-width is 800px.
+     * @default false


So... one downside about pulling defaults out of context is that react-view can't automatically determine default values anymore, so I've included them explicitly with @default JSDoc tags... not sure this is going to be great, but I do think that the props on components could use better docs in general so maybe it wouldn't be bad to implement more @default tags across the board...

Super awesome workaround/find on this! Totally agreed we could stand to use more @default doc tags for instances where either typescript or react can't infer them automatically 🙌

spalger · 2022-09-10T04:17:48Z

Welp @constancecchen, this ended up being a lot more complicated than I expected because of the const { ...rest } = props; convention in EUI and the docs site. I'm pretty confident in the state of things now, but I'm not convinced it's worth the drawbacks. That said, I think this is a pattern other parts of EUI could benefit from, maybe?

spalger · 2022-09-10T04:19:28Z

src/components/form/field_search/field_search.tsx

@@ -197,13 +198,14 @@ export class EuiFieldSearch extends Component<
  };

  render() {
+    const { defaultFullWidth } = this.context as FormContextValue;


It'd be great to avoid a type cast here, but defining the type of the context property to FormContextValue requires that we add support for declare properties in classes. I got this working in one version of this PR but it requires a lot of changes to babel configs, so I chose this route instead since I assume the class components will someday be refactored to be function components.

++, this would be a good tech debt item for sure.

I am curious though - I know one drawback of context in Class components is that they can only have one this.context. Does this have the potential of breaking if a consumer wraps their own context around a form child, e.g.

<EuiForm fullWidth> <SomeRandomContext.Provider> <EuiFieldSearch /> </SomeRandomContext.Provider> </EuiForm>

TBH i see that as pretty edge case / another tech debt reason to convert these components to function components, so mostly just thinking out loud

My understanding is that classes can only have one context for their static contextType. That doesn't limit the number of contexts that are rendered around the component. React will traverse up the graph to find the context provider that matches the contextType and connect to that, it will skip over the the SomeRandomContext in your example because it doesn't match.

Ahh, gotcha! TIL 🎓

cee-chen · 2022-09-12T21:22:58Z

src/components/form/form_control_layout/form_control_layout.tsx

+  static contextType = FormContext;
+
  render() {
+    const { defaultFullWidth } = this.context as FormContextValue;


[just thinking out loud] Does it make sense at all to let EuiFormRow also wrap or set a context value if no EuiForm parent exists? e.g., the following scenario where a consumer is not using EuiForm:

<EuiFormRow fullWidth> <EuiFieldSearch /> // inherits fullWidth from EuiFormRow, no need to declare it again </EuiFormRow>

on second thought that's probably too much of a headache / not worth the effort vs just saying "use EuiForm" haha 🙈

Yeah, I thought about the idea of wrapping the render method of all these components with <FormContext.Provider value={formContext}> so that the fullWidth value at any level would propagate downward. I think it's hard to explain that in docs though, so I decided against it.

cee-chen · 2022-09-12T21:29:28Z

I'm pretty confident in the state of things now, but I'm not convinced it's worth the drawbacks. That said, I think this is a pattern other parts of EUI could benefit from, maybe?

TBH skimming the code diff, I think the biggest annoyance is the class components. The function components feel like elegant/easy to grok changes to me.

this ended up being a lot more complicated than I expected because of the const { ...rest } = props; convention in EUI and the docs site.

Do you mind diving into this a bit more? I like the way you handled ...rest (by grabbing the form context first and making it a = undefined fallback) but is there another friction point with that that I missed?

The docs/@default changes look fine to me honestly, I think it's a totally appropriate workaround/approach and I also agree that context is a pattern we can/should/do use in a few other places in EUI (e.g. EuiDescriptionList).

@chandlerprall do you mind weighing in on this as well?

spalger · 2022-09-12T21:41:22Z

this ended up being a lot more complicated than I expected because of the const { ...rest } = props; convention in EUI and the docs site.

Do you mind diving into this a bit more? I like the way you handled ...rest (by grabbing the form context first and making it a = undefined fallback) but is there another friction point with that that I missed?

I tried several different versions of this. The first thing I tried ended up being the one that works the best I think and is the one in the PR. I feel like it would have been better if we could have just done something like const { fullWidth } = useFormContext(props) but I had to include the fullWidth declaration in the destructuring of props in order for ...rest to work, which wasn't ideal, but ...rest is important so I'm not complaining 😅.

Things I tried:

const Component: React.FC = ({ foo, fullWidth, ...rest }) => {
  ({ fullWidth } = useFormContext({ fullWidth })); // weird syntax, not great, if props are not defined in the args then they're not mutable and this wouldn't work
  // ...
}

const Component: React.FC = (props) => {
  const { foo, ...rest } = props;
  const { fullWidth } = useFormContext(props); // leaves `fullWidth` prop in `rest` so it ends up getting passed to things like `<div>`
  // ...
}

const Component: React.FC = (props) => {
  const {
    foo,
    fullWidth,
    ...rest
  } = mergeFormContext(props) // when form context has more than one property that isn't used *everywhere* those extra props get passed down in `rest`
}

chandlerprall · 2022-09-13T20:21:36Z

@spalger did you explore using an HOC to access an override from context instead? I think that would limit the consuming component changes to just being wrapped with the HOC, avoids distinguishing between function & class components, and should avoid the default/prop/context dances.

Hmm, an HOC would allow some more out-of-the-box configuration too - we recently implemented a similar idea on the description list components and explored it for the flyout, I imagine this is going to be a common thought as we go through the emotion conversion and for new components.

e.g.

export const MyComponent = withPropFromContext(_MyComponent, theContext, 'fullWidth');

cee-chen · 2022-09-13T20:36:42Z

TBH I feel fairly strongly that I would rather us spend time converting the class components to function components than creating an HOC version of this context.

avoids distinguishing between function & class components

IMO, it creates an even stronger distinction/confusion in that function components will be using context and class components will only be using props.

and should avoid the default/prop/context dances.

Just IMO calling useContext() before destructuring props is hardly a dance. It's a perfectly fine way of writing JS, and (again, IMO) more elegant than prefixing all our components with underscores.

Hmm, an HOC would allow some more out-of-the-box configuration too - we recently implemented a similar idea on the description list components and explored it for the flyout

We definitely did not implement a HOC on EuiDescriptionList, we used a context provider and useContext since all children were function components. Same for the flyout, we removed that context but it was still context-only and had no HOCs. I would have remembered because I'm not a fan of HOCs and I would have pushed against it :) HOCs feel to me like they belong in the old school class component era, and context and hooks were meant to replace them.

chandlerprall · 2022-09-14T15:33:18Z

I was thinking the HOC would be used for all components, not only classes, as a building block that could provide this pattern. Normally I also lean away from HOCs, but this one seems like a decent place, and if it was built would be re-usable in the description list case - not that we made an HOC for that one :)

The two changed lines for useFormContext + setting the default is very clean and I'm fine with leaving it as-is if we don't want to codify into more of a reusable building block. Either way I'd like to wait for @thompsongl's review on this as well.

cee-chen · 2022-09-14T16:35:54Z

if it was built would be re-usable in the description list case - not that we made an HOC for that one :)

Ahh gotcha, I misunderstood, apologies.

However, I'm still not seeing how we would reuse it for EuiDescriptionList or why we would want to. These contexts should be different (I'm not seeing why we would want to share/reuse a context for both EuiForm and EuiDescriptionList) and contain different default props - I don't find any syntactical sugar we add around "a HOC that contains context" to be more straightforward than... just using context.

I also want to add, one reason I dislike using HOCs and doing the whole 'underscore the component then export the HOC'd component' thing is that we end up with a million snapshot updates in Kibana where the component name changes to the underscored version. It's a relatively huge pain and in this case I see it as wholly unnecessary when an alternative exists (as opposed to, e.g. WithEuiTheme where an alternative does not exist because it has to be compatible with all EUI components and some components simply cannot be migrated away from a class architecture).

++ with waiting for @thompsongl to weigh in!

spalger · 2022-09-14T16:45:48Z

I really don't like HOCs and usually find they cause a lot more problems than they solve, but my primary issue with using a HOC for FormContext would be that is makes FormContext a lot less flexible.

Right now it can contain multiple values and it's not necessary for all components to use every one of those values. If we switched to a HOC then every component we wrap would get all of the props "managed" by the FormContext HOC, unless we configure it somehow and then I think it's a lot more complicated than the current implementation.

thompsongl · 2022-09-22T14:42:09Z

Sorry for the delay! I missed the notifications before he review request.

I think that static contextType is fine for this situation because the most correct approach would be to have only function components and fully embrace hooks. This seems like a temporary and non-obtrusive way to accomplish the context.
With that said, EuiDualRange (as the only exception, I think) will be a pretty involved conversion.

cee-chen · 2022-09-26T15:36:59Z

@spalger @chandlerprall @thompsongl Should we go ahead and move this PR out of draft / merge it? :)

chandlerprall · 2022-09-26T16:10:11Z

yes please!

spalger · 2022-09-26T16:10:47Z

Sweet, I'll get it freshened up today

cee-chen · 2022-09-27T21:53:51Z

@spalger just want to quickly re-ping on this - this may cause some merge conflicts for us as we move forward with converting form components to Emotion, so we're hoping to get this PR merged in sooner rather than later. Let me know if you'd like me to help out (e.g. merging main, etc) with getting this over the finish line!

spalger · 2022-09-27T21:54:22Z

Thanks for the reminder, yesterday spiraled a bit. On it now

…out-context

spalger · 2022-09-28T01:33:39Z

src-docs/src/views/form_layouts/form_layouts_example.js

@@ -130,6 +133,31 @@ export const FormLayoutsExample = {
 >
  <EuiRange fullWidth />
 </EuiFormRow>`,
+    },
+    {
+      title: 'Global Full-width',


If you've got opinions about better docs/example please feel free to re-write anything 😅 Just wanted to make sure we had an example of this actually working in several of the components.

Looks good to me! I think my only comment would be to make the title have sentence casing, per our copy guidelines

Suggested change

title: 'Global Full-width',

title: 'Global full-width',

After looking at it a bit more closely, I'm not totally sure if we should just combine the Full-width and Global full-width examples into 1 single example though - @miukimiu, any thoughts here? We can definitely address that after this PR merges though TBH

…form-layout-context

kibanamachine · 2022-09-28T14:31:42Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6229/#/forms/form-layouts#global-full-width

kibanamachine · 2022-09-28T15:44:56Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6229/

thompsongl

Thanks, Spencer!

## Summary `[email protected]` ⏩ `[email protected]` ⚠️ Note: This upgrade contains breaking changes to `EuiFlexGroup` and `EuiFlexGrid`, primarily around switching margins and negative margins to `gap`. Please do a quick QA pass of your app to scan for any issues. We're happy to help resolve minor fixes, or potentially follow up after PR merges. You can find us over in #eui! ## [`70.2.4`](https://github.com/elastic/eui/tree/v70.2.4) **Bug fixes** - Fixed visual bug in nested `EuiFlexGroup`s, where the parent `EuiFlexGroup` is responsive but a child `EuiFlexGroup` is not ([#6381](elastic/eui#6381)) ## [`70.2.3`](https://github.com/elastic/eui/tree/v70.2.3) **Bug fixes** - Fixed incorrect margins in `EuiSuperDatePicker` caused by `EuiFlex` CSS gap change ([#6380](elastic/eui#6380)) ## [`70.2.2`](https://github.com/elastic/eui/tree/v70.2.2) - `EuiButton` now accepts `minWidth={false}` ([#6373](elastic/eui#6373)) **Bug fixes** - `EuiButton` no longer outputs unnecessary inline styles for `minWidth={0}` or `minWidth={false}` ([#6373](elastic/eui#6373)) - `EuiFacetButton` no longer reports type issues when passing props accepted by `EuiButton` ([#6373](elastic/eui#6373)) - Fixed the shadow sizes of `.eui-yScrollWithShadows` and `.eui-xScrollWithShadows` ([#6374](elastic/eui#6374)) ## [`70.2.1`](https://github.com/elastic/eui/tree/v70.2.1) **Bug fixes** - Re-fixed `EuiPageSection` not correctly merging `contentProps.css` ([#6365](elastic/eui#6365)) - Fixed `EuiTab` not defaulting to size `m` ([#6366](elastic/eui#6366)) ## [`70.2.0`](https://github.com/elastic/eui/tree/v70.2.0) - Added a keyboard shortcuts popover to `EuiDataGrid`'s toolbar. This can be visually hidden via `toolbarVisibility.showKeyboardShortcuts`, but will always remain accessible to keyboard and screen reader users. ([#6036](elastic/eui#6036)) - `EuiScreenReaderOnly`'s `showOnFocus` prop now also shows on focus within its children ([#6036](elastic/eui#6036)) - Added `onFocus` prop callback to `EuiSuperDatePicker` ([#6320](elastic/eui#6320)) **Bug fixes** - Fixed `EuiSelectable` to ensure the full options list is re-displayed when the search bar is controlled and cleared using `searchProps.value` ([#6317](elastic/eui#6317)) - Fixed incorrect padding on `xl`-sized `EuiTabs` ([#6336](elastic/eui#6336)) - Fixed `EuiCard` not correctly merging `css` on its child `icon`s ([#6341](elastic/eui#6341)) - Fixed `EuiCheckableCard` not setting `css` on the correct DOM node ([#6341](elastic/eui#6341)) - Fixed a webkit rendering issue with `EuiModal`s containing `EuiBasicTable`s tall enough to scroll ([#6343](elastic/eui#6343)) - Fixed bug in `to_initials` that truncates custom initials ([#6346](elastic/eui#6346)) - Fix bug in `EuiCard` where layout breaks when `horizontal` and `selectable` are both passed ([#6348](elastic/eui#6348)) ## [`70.1.0`](https://github.com/elastic/eui/tree/v70.1.0) - Added the `hint` prop to the `<EuiSearchBar />`. This prop lets the consumer render a hint below the search bar that will be displayed on focus. ([#6319](elastic/eui#6319)) - Added the `hasDragDrop` prop to `EuiPopover`. Use this prop if your popover contains `EuiDragDropContext`. ([#6329](elastic/eui#6329)) **Bug fixes** - Fixed `EuiButton`'s cursor style when the button is disabled ([#6323](elastic/eui#6323)) - Fixed `EuiPageTemplate` not recognizing child `EuiPageSidebar`s/`EuiPageTemplate.Sidebar`s with `css` props ([#6324](elastic/eui#6324)) - Fixed `EuiBetaBadge` to always respect its `anchorProps` values, including when there is no tooltip content ([#6326](elastic/eui#6326)) - Temporarily patched `EuiModal` to not cause scroll-jumping issues on modal open ([#6327](elastic/eui#6327)) - Fixed buggy drag & drop behavior within `EuiDataGrid`'s columns & sorting toolbar popovers ([#6329](elastic/eui#6329)) - Fixed `EuiButton` not correctly passing `textProps` for children inside fragments or i18n components ([#6332](elastic/eui#6332)) - Fixed `EuiButton` not correctly respecting `minWidth={0}` ([#6332](elastic/eui#6332)) **CSS-in-JS conversions** - Converted `EuiTabs` to Emotion ([#6311](elastic/eui#6311)) ## [`70.0.0`](https://github.com/elastic/eui/tree/v70.0.0) - Added the `enabled` option to the `<EuiInMemoryTable />` `executeQueryOptions` prop. This option prevents the Query from being executed when controlled by the consumer. ([#6284](elastic/eui#6284)) **Bug fixes** - Fixed `EuiOverlayMask` to set a `[data-relative-to-header=above|below]` attribute to replace the `--aboveHeader` and `--belowHeader` classNames removed in its Emotion conversion ([#6289](elastic/eui#6289)) - Fixed `EuiHeader` CSS using removed `EuiOverlayMask` class modifiers ([#6293](elastic/eui#6293)) - Fixed `EuiToolTip` not respecting reduced motion preferences ([#6295](elastic/eui#6295)) - Fixed a bug with `EuiTour` where passing any `panelProps` would cause the beacon to disappear ([#6298](elastic/eui#6298)) **Breaking changes** - `@emotion/css` is now a required peer dependency, alongside `@emotion/react` ([#6288](elastic/eui#6288)) - `@emotion/cache` is no longer required peer dependency, although your project must still use it if setting custom cache/injection locations ([#6288](elastic/eui#6288)) **CSS-in-JS conversions** - Converted `EuiCode` and `EuiCodeBlock` to Emotion; Removed `euiCodeSyntaxTokens` Sass mixin and `$euiCodeBlockPaddingModifiers`; ([#6263](elastic/eui#6263)) - Converted `EuiResizableContainer` and `EuiResizablePanel` to Emotion ([#6287](elastic/eui#6287)) ## [`69.0.0`](https://github.com/elastic/eui/tree/v69.0.0) - Added support for `fullWidth` prop on EuiForm, which will be the default for all rows/controls within ([#6229](elastic/eui#6229)) - Added support for `onResizeStart` and `onResizeEnd` callbacks to `EuiResizableContainer` ([#6236](elastic/eui#6236)) - Added optional case sensitive option matching to `EuiComboBox` with the `isCaseSensitive` prop ([#6268](elastic/eui#6268)) - `EuiFlexItem` now supports `grow={0}` ([#6270](elastic/eui#6270)) - Added the `alignItems` prop to `EuiFlexGrid` ([#6281](elastic/eui#6281)) - Added `filter`, `filterExclude`, `filterIgnore`, `filterInclude`, `indexTemporary`, `infinity`, `sortAscending`, and `sortDescending` glyphs to `EuiIcon` ([#6282](elastic/eui#6282)) **Bug fixes** - Fixed `EuiTextProps` to show the `color` type option `inherit` as default ([#6267](elastic/eui#6267)) - `EuiFlexGroup` now correctly respects `gutterSize` when responsive ([#6270](elastic/eui#6270)) - Fixed the last breadcrumb in `EuiBreadcrumbs`'s `breadcrumbs` array not respecting `truncate` overrides ([#6280](elastic/eui#6280)) **Breaking changes** - `EuiFlexGrid` no longer supports `columns={0}`. Use `EuiFlexGroup` instead for normal flex display ([#6270](elastic/eui#6270)) - `EuiFlexGrid` now uses modern `display: grid` CSS ([#6270](elastic/eui#6270)) - `EuiFlexGroup`, `EuiFlexGrid`, and `EuiFlexItem` now use modern `gap` CSS instead of margins and negative margins ([#6270](elastic/eui#6270)) - `EuiFlexGroup` no longer applies responsive styles to `column` or `columnReverse` directions ([#6270](elastic/eui#6270)) **CSS-in-JS conversions** - Converted `EuiFlexGroup`, `EuiFlexGrid`, and `EuiFlexItem` to Emotion ([#6270](elastic/eui#6270)) ## [`68.0.0`](https://github.com/elastic/eui/tree/v68.0.0) - Added `beta` glyph to `EuiIcon` ([#6250](elastic/eui#6250)) - Added `launch` and `spaces` glyphs to `EuiIcon` ([#6260](elastic/eui#6260)) - Added the `fallbackDestination` prop to `EuiSkipLink`, which accepts a string of query selectors to fall back to if the `destinationId` does not have a valid target. Defaults to `main` ([#6261](elastic/eui#6261)) - `EuiSkipLink` is now always an `a` tag to ensure that it is always placed within screen reader link menus. ([#6261](elastic/eui#6261)) **Bug fixes** - Fixed `EuiSuperDatePicker` not correctly merging passed `className`s ([#6253](elastic/eui#6253)) - Fixed `EuiColorStops` not correctly merging in passed `data-test-subj`s, `style`s, or `...rest` ([#6255](elastic/eui#6255)) - Fixed `EuiResizablePanel` incorrectly passing `style` to the wrapper instead of the panel. Use `wrapperProps.style` to pass styles to the wrapper. ([#6255](elastic/eui#6255)) - Fixed custom `onClick`s passed to `EuiSkipLink` overriding `overrideLinkBehavior` ([#6261](elastic/eui#6261)) **Breaking changes** - Removed `inherit` and `ghost` color from `EuiListGroupItem` ([#6207](elastic/eui#6207)) - Changed default color to `text` instead of `inherit` ([#6207](elastic/eui#6207)) **CSS-in-JS conversions** - Converted `EuiListGroup` and `EuiListGroupItem` to Emotion; Removed `$euiListGroupGutterTypes`, `$euiListGroupItemColorTypes` and `$euiListGroupItemSizeTypes`; ([#6207](elastic/eui#6207)) - Converted `EuiBadgeGroup` to Emotion ([#6258](elastic/eui#6258)) - Converted `EuiBetaBadge` to Emotion ([#6258](elastic/eui#6258)) - Converted `EuiNotificationBadge` to Emotion ([#6258](elastic/eui#6258)) Co-authored-by: Elizabet Oliveira <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

[EuiForm] specify default fullWidth with root component

564e188